[SPARK-57846][SQL] Add the try_make_time function#56932
Conversation
MaxGekk
left a comment
MaxGekk
left a comment
There was a problem hiding this comment.
1 blocking, 0 non-blocking, 1 nits.
Clean, faithful mirror of the try_* datetime-constructor pattern (TryEval(MakeTime(...)), like try_to_time). Implementation, registry, API surface, docs, and end-to-end tests are all correct and consistent. One blocking versioning fix.
Correctness (1)
timeExpressions.scala:645,functions.scala:904,builtin.py:25589:@since/versionaddedis4.1.0, but v4.1.0 is already released and does not contain this new function — should bebranch-4.x's4.3.0. See inline.
Nits: 1 minor item (see inline comments).
| NULL | ||
| """, | ||
| group = "datetime_funcs", | ||
| since = "4.1.0") |
There was a problem hiding this comment.
try_make_time is a new function, but v4.1.0 is already released and does not contain it (it's absent from the v4.1.0 function registry). since should be the version this first ships in — branch-4.x = 4.3.0 (make_time/try_to_time legitimately say 4.1.0 because they shipped in that release; the value was likely copied from make_time).
| since = "4.1.0") | |
| since = "4.3.0") |
If this is intended to be master-only (unusual for a new user-facing function), use 5.0.0 instead.
There was a problem hiding this comment.
Fixed - updated since to 4.3.0 in all three locations (ExpressionDescription, Scala API @SInCE, PySpark versionadded), since try_make_time is new in branch-4.x and absent from the released 4.1.0. Also changed the unit test to construct TryMakeTime(...) directly so the new expression/builder is exercised.
| * @param second | ||
| * the second to represent, from 0 to 59.999999 | ||
| * @group datetime_funcs | ||
| * @since 4.1.0 |
There was a problem hiding this comment.
Same versioning issue — update the Scala API @since to match (4.3.0, the branch-4.x version this ships in).
| * @since 4.1.0 | |
| * @since 4.3.0 |
| Try to create time from hour, minute and second fields. | ||
| The function returns NULL on invalid inputs. | ||
|
|
||
| .. versionadded:: 4.1.0 |
There was a problem hiding this comment.
Same versioning issue — update the Python versionadded to match (4.3.0). (The Connect builtin.py forwards __doc__, so no separate change is needed there.)
| .. versionadded:: 4.1.0 | |
| .. versionadded:: 4.3.0 |
| ) | ||
| } | ||
|
|
||
| test("creating values of TimeType via try_make_time") { |
There was a problem hiding this comment.
Nit (non-blocking): this test builds TryEval(MakeTime(...)) directly, so despite its title it doesn't instantiate the new TryMakeTime expression or TryMakeTimeExpressionBuilder. The class/builder are covered end-to-end by TimeFunctionsSuiteBase and the SQL golden, so coverage isn't missing — but constructing new TryMakeTime(...) here would make the unit test match its title and directly cover the new expression's plumbing (including the builder's arg-count error path, which nothing currently exercises).
…st TryMakeTime directly
|
@MaxGekk Addressed the review comments: added a TryMakeTime wrapper so the display name is try_make_time, added the PySpark binding, fixed the doctest, and set the version to 4.3.0. Could you take another look when you get a chance? |
2 participants
What changes were proposed in this pull request?
Adds the
try_make_time(hour, minute, second)function, the error-safe counterpart tomake_time: it returns the same result on valid input andNULL(instead of throwing) on invalid input. It is implemented asTryEval(MakeTime(...)), mirroringtry_make_timestamp. Registered in the function registry, exposed viafunctions.scala, and documented (ANSI-compliance try-functions list +sql-expression-schema).Why are the changes needed?
make_timethrows on invalid components (e.g.hour = 25). Users want a null-returning variant consistent with the othertry_*datetime constructors (try_make_timestamp, etc.).Does this PR introduce any user-facing change?
Yes - a new function
try_make_time.How was this patch tested?
Unit tests (
TimeExpressionsSuite), integration (TimeFunctionsSuiteBase), SQL-level (time.sql), and Connect plan-generation goldens: valid input equalsmake_time, invalid input returnsNULL.Was this patch authored or co-authored using generative AI tooling?
Authored with assistance by Claude Opus 4.8.